-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[종근] 배민찬 3단계(프로모션) 구현 완료 + 1~3단계 프론트 구현 #11
base: econo-jonggeun
Are you sure you want to change the base?
Conversation
1. id 찾기 기능 2. password 찾기 기능 3. 찾기 실패시 에러 메세지
1. 임시비밀번호 생성 기능 테스트 코드 구현(실패) 2. 임시비밀번호 생성 기능 코드 구현(통과)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
내용이 많군요 이따 추가로 하겠습니다.
리뷰중에 굳이 문제가 없는데 이유를 물어본 리뷰도 있습니다.
코드를 쓸때 한줄 한줄에 작성자의 의도가 있어야 한다고 생각해서 굳이 물어본 것이니
그냥 코드 작성한 이유를 간단간단히 설명해 주시면 좋을 것 같습니다.
@Configuration | ||
public class WebMvcConfig implements WebMvcConfigurer { | ||
public abstract class WebMvcConfig implements WebMvcConfigurer { | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
abstract로 선언한 이유는 무엇인가요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
profile 공통 설정을 가진 부모 클래스를 만들면서 초기에 선언 했었는데 추상 메소드를 따로 구현안해서 필요없는 부분이 되버린 걸 안 지웠네요
@Override | ||
public void addArgumentResolvers(List<HandlerMethodArgumentResolver> argumentResolvers) { | ||
argumentResolvers.add(loginUserArgumentResolver()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 아직 구분을 못해서
위의 두 메서드가 하는 역할이 어떻게 다른지 궁금합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@loginuser 라는 어노테이션을 커스텀해 사용하기 위해 작성한 부분입니다.
HandlerMethodArgumentResolver를 상속받은 loginUserArgumentResolver는 @loginuser를 위한 바인딩 설정을 하는 역할이고,
addArgumentResolver는 만든 resolver를 argumentREsolvers 리스트에 추가함으로써 어노테이션이 동작하게 하는 역할인데 솔직히 아직 와닿는 부분이 아니라서 학습이 필요한 부분입니다.
@Configuration | ||
@Profile({ "local", "dev", "prod" }) | ||
static class NotTestWebMvcConfig extends WebMvcConfig { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드의 의도는 test 스코프가 아닌 다른 scope일때 아래를 적용하라는 것 같은데
이 코드를 아예 작성하지 않았을때와 어떤 차이가 있나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 코드를 작성하지 않으면 test 스코프 밖에 남지 않아서 다른 프로필에서는 동작하지 않을 것 같습니다. 나중에 시도하고 직접 확인해 보겠습니다.
@@ -27,7 +28,7 @@ | |||
@Column(nullable = false, name = "password") | |||
private String password; | |||
|
|||
@Column(nullable = false, name = "name") | |||
@Column(nullable = false, name = "user_name") | |||
private String name; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
필드의 변수명으로 칼럼명을 추론하도록 둘 수 있는데
굳이 필드명에서 자동 생성되는 이름 말고 다른 칼럼명을 부여해 준 이유가 궁금합니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그에서 나중에 다른 엔티티의 칼럼명과 혼동이 올까봐 가독성을 고려해서 변경하게 되었습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
조금 더 이야기를 해볼까요
저는 name=""으로 선언된 칼럼 속성이 매우 불필요한 반복작업 처럼 느껴져서요.
헷갈릴 가능성이 있는 다른 엔티티의 예시가 있을까요?
Account 객체의 name필드인데, 저는 이게 다른 엔티티와 쉽게 혼동이 생길거라고 생각되지는 않습니다!
@@ -9,4 +9,5 @@ | |||
public interface AccountRepository extends JpaRepository<Account, Long> { | |||
|
|||
Optional<Account> findByEmail(String accountId); | |||
Optional<Account> findByName(String name); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name과 email이 중복된 경우가 있을땐 어떻게 되나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스단에서 email에 대한 중복 예외 처리 구현했는데 디비에서 unique 처리하도록 수정하겠습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
제가 name이 여러개 있을경우
Optional<Account> findByName(String name);
위 코드에서 id가 젤 빠른 한개만 가져온다고 설명드렸었는데,
지금 생각해보니 확실하지 않은 정보를 전달한 것 같네요.
그말은 잊어버리고 궁금해질때 직접 찾아보시길 권합니다~
this.children = null; | ||
|
||
return this.build(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
현재 이 메서드를 사용하기 위해서, build()메서드, Category를 파라미터로 받는 생성자 까지 2개의 메서드를 추가로 사용해야 하네요.
빌더 패턴과는 달리 원하는 필드를 자유롭게 하나씩 설정 할 수 있는 장점도 없어져 있어서, 다른 방식을 사용하는게 좋을 듯 합니다.
기본 값을 가진 객체 생성 + 특정 이름을 주고 싶은 의도라면 적절한 디자인 패턴이 존재합니다.
객체 생성과 관련된 디자인 패턴 한번 정리해보시면 좋을 것 같아요
너무 시간쓰지 마시고 적당히 찾아서 안나오면 그냥 물어보시면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
빌더 패턴을 이해하고, 변화시켜 응용하려고 하신점은 훌륭하신것 같습니다
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
정적 팩토리 메서드 알아보고 반영하겠습니다
@Column(nullable = false, name = "final_price") | ||
private int finalPrice; | ||
|
||
public boolean isType(PromotionType promotionType) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 이름은
[짧은 것 << 의도가 잘 나타나는 것] 순으로 우선순위를 주는게 좋을 것 같아요
조금 길더라도 isSameType과 같은식으로 누가봐도 중의적으로 해석 되지 않을 메서드 명을 사용하면 좋을 것 같습니다.
isType 이라고 해서 전달한 타입이 올바른 타입 객체인지 유효성 검증하는 메서드인가 잠깐 생각했습니다.
|
||
@Pattern(regexp = RegexUtil.REGEX_PHONE_NUMBER) | ||
private String phoneNumber; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 DTO는 불변객체가 아닐 이유가 없는 객체로 보입니다. (데이터가 많지 않네용)
Setter를 두면, 다른 클라이언트가 사용시에 동기화도 보장 할 수 없고, 불변성이 보장되지도 않는 단점이 있습니다.
final을 통해서 필드들의 불변성이 보장되는 불변 객체로 만들면 좋을 것 같습니다. (Setter는 꼭 필요할 때만 쓰고, 안쓸 수 있다면 안써야합니다)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
어떻게 수정하실건지도 들려주세요!
불변 객체에 대해 찾아보시길 권합니다!
String name; | ||
|
||
@Pattern(regexp = RegexUtil.REGEX_PHONE_NUMBER) | ||
String phoneNumber; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위와 같은듯 합니다
|
||
public class UnAuthorizedException extends RuntimeException { | ||
public UnAuthorizedException(String s) { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
super(s)가 없는 이유가 있나요?.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
실수로 빼먹은 것 같습니다. super(s)는 문자열 s를 파라미터로 받는 부모 클래스인 runtimeExeption의 생성자를 호출합니다.
|
||
public class BasicAuthInterceptor extends HandlerInterceptorAdapter { | ||
private static final Logger log = LoggerFactory.getLogger(BasicAuthInterceptor.class); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
static을 사용할 이유가 있을까요?
static을 사용했을때 장단점이 어떤게 있을까요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
사실 관습처럼 생각없이 사용해 왔었던 부분입니다.(반성)
static을 선언하면 해당 객체는 static 메모리에 할당이 되어 프로그램이 시작되고 종료될 때까지 살아있게 됩니다. 별도의 객체를 생성하지 않고 사용할 수 있고 객체를 공유할 수 있지만, static이 선언된 변수 값을 변경하게 되면 변경된 값이 다른 곳에도 영향을 주기 때문에 주의해야합니다.
제가 작성한 코드대로면 클래스마다 해당 클래스 정보를 담는 logger들이 존재하기 때문에 logger들 간 영향을 주면 안되기 때문에 static을 제외하는게 맞겠네요
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 위의 내용은 다 맞는말이지만 전 두번째 내용에 대해선 약간 다르게 생각합니다.
제가 작성한 코드대로면 클래스마다 해당 클래스 정보를 담는 logger들이 존재하기 때문에 logger들 간 영향을 주면 안되기 때문에 static을 제외하는게 맞겠네요
이 내용을 조목조목 따지려면, final로 선언된 log 인스턴스가 불변 객체인지 확인해보고 하는등의 논의를 해야겠지만
여기선 그것보다, static 으로 선언하는 이유는 여러 인스턴스에서 공통적으로 쓰이는 값을 클래스 변수로 바꿔서, 모든 인스턴스가 하나의 static한 데이터를 바라보게 함으로써 메모리 사용을 줄이는 효과가 가장 클거라고 생각합니다.(그외 이유도 많음)
다만 스프링에서는 빈으로 명시된 클래스는 하나만 생성해서 관리하기때문에 굳이 static을 사용할 필요가 없다고 생각합니다.
그 이유는 static을 공부해서, static으로 선언된 데이터가 JVM상에 어디 저장되는지, 단점이 무엇인지 등등을 공부하면 알수 있습니다
|
||
import javax.servlet.http.HttpSession; | ||
|
||
public class HttpSessionUtils { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util에 해당하는 클래스는
다른 코드 사용자(클라이언트)가 다른 방식으로 이 유틸 클래스를 사용하지 않도록 해줘야 합니다.
이를테면 HttpSessionUtils의 인스턴스를 만들어서 사용하게 될 수도 있는데 그런 경우를 막는 코드를 추가하면 좋을 듯 합니다.
public class RandomPasswordGenerator { | ||
private static final int PASSWORD_LENGTH = 8; | ||
private static char[] defaultList = new char[]{'0', '1', '2', '3', '4', '5', '6', '7', '8', '9', | ||
'A', 'B', 'C', 'D', 'E', 'F', 'G', 'H', 'I', 'J', 'K', 'L', 'M', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final을 통해 불변성을 보장하지 않을 이유가 없어보입니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 Generator객체는 util처럼 동작하네요, Generator이라고 명명하면 저는 당연히 인스턴스로 만들어서 사용 할 것 같습니다.
어느방향이든 둘중 한 방향으로 일관되도록 수정하면 좋을 것 같습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
util의 성격을 염두해 두고 구현했습니다. 그런데 이 기능이 다른 곳에서 사용되지 않을 것 같아 서비스단 메소드로 옮겨야 할 것 같단 생각이 들었습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
음 전 지금의 클래스 단위가 매우 좋아보입니다.
누가봐도 랜덤한 패스워드를 생성하기위한 클래스로 이렇게 기능이 명확한 클래스는 많이 없을것 같습니다.
말씀하신때로, 기능이 다른곳에서 사용되지 않는다면, util의 성격을 빼고, 해당 서비스 단의 support를 위한 클래스나, 해당 도메인에 핻당하는 패키지로 패키지를 변경하는게 더 맞을듯 합니다.
return account; | ||
} | ||
|
||
public Account login(LoginDTO loginDTO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
메서드 명때문에 session 등록하는부분이 어디있는지 찾아다녔네요.
메서드 명이 잘못된 정보를 주고있습니다.
Account account = accountRepository.findByName(findingIdDTO.getName()) | ||
.orElseThrow(() -> new NotFoundAccountException(ExceptionMessages.NO_ACCOUNT_WITH_SUCH_INFO)); | ||
|
||
if (!account.hasSamePhoneNumber(findingIdDTO.getPhoneNumber())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
서비스 코드에서 phoneNumber처럼 하나씩 따로 검사하는 것 보다는,
Account객체에서 주어진 정보를 체크하는 메서드를 하나 만드는게 좋을 듯 합니다.
account에 검증할 필드가 추가되도 여기로 와서 검증 로직을 추가할 필요없이
account객체 내의 정보 체크 메서드에서 검증 로직을 추가해주기만 하면 됩니다.
객체는 수정에 닫혀있어야 한다고 하네요
return account.getEmail(); | ||
} | ||
|
||
public String findPassword(FindingPasswordDTO findingPasswordDTO) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배민찬 고객 입장에서는 비밀번호 찾기 이지만,
메서드의 기능은 검증후에 새로운 패스워드로 변경하는 로직인것 같네요.
메서드 명은 메서드의 내용을 반영하는 이름이 좋을 것 같습니닫.
public class PromotionService { | ||
|
||
private final PromotionRepository promotionRepository; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
의존성이 주입되나요?
@requiredargsconstructor는 어떤동작을 하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
final이나 @NotNull이 선언된 필드를 파라미터로 받는 생성자를 만들어주는 어노테이션으로 생성자 의존 주입 시에 직접 생성자를 만들어 주지 않고도 기능 구현이 가능합니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
좋은 정보입니다.
다만 저도 이부분에서 의존주입이 제대로 되나? 라고 생각했듯이.
@RequiedArgsConstructor이 final과 @NotNull인 필드도 생성자 인자로 만들어 주는걸 알지못하는 다른사람들도 이부분에 와서 저와 같은 고민을 할거라고 생각합니다.
코드가 짧고 지저분한 생성자 주입 코드가 없어진건 좋지만, 같은 코드를 보는 팀원이 모두 이부분에 와서 멈칫 한후에 해당 내용에 대한 설명을 들어야 하는것은 전 좋은 코드라고 생각하긴 어려울 것 같습니다.
약속된 형태는 특별한 이유가 없다면, 팀원들이 보고 당연하게 넘어갈수 있도록 약속한 형태 그대로 사용하는 것이 더 좋을 것 같습니다.
} | ||
|
||
public List<Promotion> findSubDishList() { | ||
return findAll().stream().filter(dish -> dish.isType(SUB_DISH)).collect(Collectors.toList()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
프로모션 데이터가 많아지면, 필요하지 않은 그 많은 데이터를 findAll()로 받아오는데 굳이 그럴 필요가 없어보이네요.
성능 많이 단축 가능해보입니다!
return ResponseGenerator.generateResponseEntity(HttpStatus.FOUND); | ||
HttpHeaders headers = new HttpHeaders(); | ||
headers.setContentType(MediaType.APPLICATION_JSON); | ||
return new ResponseEntity<>(headers, HttpStatus.FOUND); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestController에서 response의 http content type 기본 값이 Application Json입니다.
단지 HttpStatus만 변경하고 싶은거라면 여기에 해당하는 어노테이션이 존재하니까 이용하는것도 좋을 것 같습니다.
그게 아니더라도 default Header를 세팅하는 메서드는 유틸 또는 해당 클래스로 만들어 두면 재사용에 도움이 될 듯 합니다.
|
||
@PostMapping("/member/find/request") | ||
public ResponseEntity<String> findId(@Valid @RequestBody FindingEmailDTO findingEmailDTO) { | ||
String foundId = accountService.findId(findingEmailDTO); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ID는 identifier와 헷갈려서 userId로 많이 씁니다.
return ResponseGenerator.generateResponseEntity(HttpStatus.OK); | ||
HttpHeaders headers = new HttpHeaders(); | ||
headers.setContentType(MediaType.APPLICATION_JSON); | ||
return new ResponseEntity<>(headers, HttpStatus.OK); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
위의 경우들을 봤을때 default Rest Response Header를 생성하는 메서드를 만들어두면 유용하게 쓸 수 있을 것 같네요.
let fourthDot = dots.item(3); | ||
let fifthDot = dots.item(4); | ||
let el = document.querySelector(".img-box"); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
아이템이 추가될경우를 가정해서 작성 할 수 있을 것 같네요.
@@ -23,7 +25,7 @@ | |||
import static org.mockito.Mockito.when; | |||
|
|||
|
|||
@RunWith(MockitoJUnitRunner.class) | |||
@RunWith(MockitoJUnitRunner.Silent.class) | |||
@SpringBootTest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
처음보네요. 무슨역할을 하나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
배포 당시 unnecessary mockito stubbings 에러가 발생해서 해결책을 찾던 도중 시도해본 흔적입니다. mockito에서 테스트 코드에서 불필요하거나 사용하지 않는 테스트 코드를 엄격하게 감지해 클린한 테스트코드 유도하려고 이런 에러를 유도한다네요. 그걸 무시하려고 silent를 선언했던거 같은데... 이부분 다시 알아보도록 하겠습니다
프론트 - 프로모션 transition 전환 효과
Spring profile를 적용해 배포 환경별 설정
EC2, Travis CI, AWS S3, AWS CodeDeploy를 적용해 배포 자동
프론트 - 로그인 후 회원가입, 로그인 버튼 가리기. 로그아웃 버튼 구현 [Bug] 로그인해도 로그인 버튼이 등장합니다. #9
아이디 찾기, 비밀번호 찾기(임시비밀번호 발급) 구현
카테고리 db에서 받아오기
프로모션 데이터는 멘토님과 상의한 결과 나중에 카테고리 컨텐츠 구현 단계에서 함께 구현하기로 했습니다.